Skip to content

fork: delete four outside-started upstream workflows and keep them deleted - #14

Merged
scuttletoby merged 2 commits into
mainfrom
fork/remove-outside-workflows
Nov 12, 2026
Merged

fork: delete four outside-started upstream workflows and keep them deleted#14
scuttletoby merged 2 commits into
mainfrom
fork/remove-outside-workflows

Conversation

@scuttletoby

@scuttletoby scuttletoby commented Nov 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

What

The fork's sweep (fork-workflows.sh) turns off upstream workflows only after GitHub lists them. That means it cannot stop the first run of a workflow that an outside event starts. On 2026-11-12 the owner decided to delete four such upstream workflows from the fork:

File Why outside events can start it
.github/workflows/mobile-eas-preview-fork.yml pull_request_target: it runs main's copy with a write token
.github/workflows/desktop-macos-preview.yml owner decision
.github/workflows/thread-transfer-report.yml workflow_run on a workflow named "CI", and an outside PR can add a workflow with that name
.github/workflows/mobile-fingerprint-label.yml same workflow_run "CI" pattern

How they stay deleted

  • .github/fork-removed-workflows.txt is the list, kept in config rather than in a hand-kept patch. fork-follow.sh reads it from the series head.
  • Fail closed: a missing list, or a line that is not .github/workflows/<name>.yml (or is a fork-* file), stops the plan red and leaves the series as it is.
  • Rebase conflicts: if every conflicting path in a stop is on the list (for example, upstream edited a file the fork deletes), the plan deletes those paths and continues. A stop that also touches an unlisted path still fails the way it does now, naming that path and aborting the rebase.
  • Files upstream brings back: after the rebase, the plan deletes any listed file the candidate still has, in one more commit (fork: delete upstream workflows listed in …). This happens when the fork's deleting patch was dropped as empty because upstream deleted the file itself, and upstream later re-adds it.
  • Same base: if the series head still has a listed file, the plan refuses. On that path the candidate is the series head itself, so the plan does not edit it.
  • General case: if a new upstream release adds a workflow that someone outside the fork can start, the plan stops red and names the file, unless the list names it (then it is deleted). The plan stops the same way when it cannot read a new file's triggers.

    • The events that count, from GitHub's full event list:

      • Started by anyone directly: pull_request_target, pull_request_review, pull_request_review_comment (anyone can review a PR from a fork/* branch, and that branch's copy then runs with the repository's token), issues, issue_comment, discussion, discussion_comment, watch, fork, gollum (the fork's wiki is on).
      • Started indirectly: workflow_run (an outside PR can add a workflow with the watched name), and check_run, check_suite, status and deployment_status (apps post these on an outside PR's commits).
      • Not counted: pull_request from another fork gets a read-only token and no secrets. The other events need write access or more.

    • The triggers are read by the new dependency-free fork-workflow-triggers.cjs. It reads only the top-level on:, so issues: write under permissions or a job named issues does not count.

    • Any pull_request_target or workflow_run word anywhere in the file also counts.

    • The reader reads plain YAML only. Anything else counts as unreadable, so the plan stops:

      • a line break other than \n or \r\n (a lone \r, U+0085, U+2028, U+2029), or a tab in the indent;
      • an unindented line that is not a plain key: (or "on":), --- or .... This refuses on :, escaped or quoted keys, ? on, directives, a root-level list and a BOM;
      • a quote left open at the end of a line (outside block scalars);
      • a bracket list over several lines unless its later lines are indented more than its first (a last line of only closing brackets may line up with it, which is how upstream writes needs:), or one that never closes;
      • on: twice, several YAML documents, no on:;
      • under on:: a flow mapping, a list over lines, an anchor, alias or tag, a block scalar, <<, uneven indent, or a name that is not an event name.

      So a decoy on: cannot hide in a multi-line string or list. Over all 30 workflow files on fork main and all 26 on upstream main, the reader's output matches PyYAML's on: keys.

  • fork-contracts.test.py gets a new removed section. It checks that the list exists, that every line is an upstream workflow path that is not in the tree, and that the four files above are on it. Each of these has a broken copy that must fail.

What happens when this runs again, stops half-way, or meets existing, old or empty state?

  • A follow re-run rebuilds the same tree: the conflict resolution and the re-delete commit are deterministic in content. When an earlier run published the candidate and then stopped before moving the branch, the next run finds that release by tree and advances to it (tested). A re-run after a red stop meets the same state and stops again with the same reason. Nothing is pushed or published before the plan passes.
  • Interrupted between steps: the plan itself pushes nothing; it works in the job's own checkout. A failed rebase is aborted (git rebase --abort), so the series is left as it is. Later steps (candidate push, release, branch move) are unchanged and already handle re-runs.
  • Upstream renames a listed file: git reports a rename/delete conflict on the new name. That name is not on the list, so the plan stops red, names it and aborts the rebase (tested). If the rename also changes the file enough that git sees a delete plus an add, the fork's delete applies cleanly. The "new" file then goes through the general check: red if outside events can start it, otherwise it lands and the sweep turns it off like any upstream workflow.
  • The list is empty (comment lines only): nothing is deleted and the plan goes on (tested). Files the series already deleted stay deleted until upstream edits one. That edit is then a modify/delete conflict on an unlisted path, so the plan stops red rather than bring the file back.
  • The list is missing or malformed: red, and nothing is planned (tested for missing, a bare name, a fork-* path, a path with a space, and ../).
  • Old state: fork main today has no list. The follow reads the list from the series, so the first follow after this merges has it. A follow run queued before this merges does not use its own commit's script: the plan job checks out SERIES_BRANCH (fork-follow.yml), so it runs main's current fork-follow.sh. Before this merges that script has no list logic, hits a modify/delete conflict on these files and stops red, as today. After it merges, the run uses this logic. Either way it does not ship them.

Tests (red on fork main, green here)

The tests never reach GitHub: fork-follow.test.sh runs under fork-test-gh.sh (no GH_TOKEN/GITHUB_TOKEN, .invalid GH_HOST, stub gh), and the plan uses only local git remotes.

  • bash .github/scripts/fork-follow.test.sh: all 131 checks pass here. Run against fork main's scripts (FORK_FOLLOW_DIR=<main's .github/scripts>), 24 fail: every new plan check except the three that cover unchanged behaviour (an empty list goes on, a push-only new workflow goes on, a rename is refused, which main also does through its plain conflict).
  • node --test .github/scripts/fork-workflow-triggers.test.cjs: 13 tests of the reader. They cover every shape it reads and every refusal, including upstream's own forms (CRLF, block scalars with quotes in them, a needs: list over lines).
  • Mutations, each red: I broke each of these by hand, one at a time, and the named test failed.
    • Reader, fork-workflow-triggers.test.cjs (1 test each): the odd line break check, the tab check, the plain-key: rule, the open-quote rule, the bracket-indent rule, the never-closed bracket, the aligned closing line, on: twice, several documents, the unknown on: forms, <<, uneven indent, empty on:, the event-name check, a nested list under on:, quotes only at a token start, ASCII-only indent, the block-scalar skip, the block's key and dash indent, the block's end, and exit 2 on error.
    • Plan, fork-follow.test.sh: the CR strip on list lines (1 fails), the plain-text pull_request_target|workflow_run backstop (1), and the review and gollum events (3).
    • Removing only the open-quote rule leaves the plan suite green, because the plan-level decoy case is also refused by the plain-key: rule. The reader test covers the quote rule on its own.
  • python3 .github/scripts/fork-contracts.test.py: all pass here. Run on fork main's tree with this test file, 3 fail (the list is missing, the four files are not on it, and the "listed file still in the tree" mutant).
  • Also run, all green: node .github/scripts/fork-ci-tests.cjs (every listed fork test: fork-release.test.sh, fork-secret-scan.test.sh, fork-release-workflow.test.py, the fork-*.test.cjs tests), node .github/scripts/fork-tests-drift.cjs, actionlint 1.7.12 (the pinned, checksummed release) on fork-*.yml, vp fmt --check and vp lint on .github, and knip's file check. Without the new test file knip reports fork-workflow-triggers.cjs as unused, as CI did; with it, it does not.

Lint fixes

  • knip "unused file": fork-workflow-triggers.cjs now exports readTriggers, and fork-workflow-triggers.test.cjs requires it. Every other fork-*.cjs script is kept in use this way, through knip's .github/**/*.test.cjs entry. The test is listed in .github/fork-ci-tests.txt.
  • vp check: knip failed first, so vp check never ran on this PR. fork-schema-check.test.cjs (from PR fork: outside PRs run nothing, one checkout without actions/checkout, contract tests #12, which merged without a Fork tests run) has an unused cmd parameter that vp lint reports as an error. It is dropped here.

Checks on this head

  • "No database schema changes" is red by design. The one-line vp lint fix touches fork-schema-check.test.cjs, and the check refuses any change to its own files ("Only the owner may do that: the check stays red, and the owner merges past it"). Reverting the fix would make this check green but leave vp check red in Lint, so the fix stays.
  • Fork tests has not run on this head. fork-tests.yml is disabled in the fork (since 17:33Z), so no run started for the push. Once it is enabled, a new push or a close and reopen of this PR starts it.

Not done

Follow-ups (the fork has Issues off, so they are listed here)

  • Edited upstream workflows are not checked. The general check reads only files a release adds. Eight upstream files in the fork have never been listed by GitHub, so the sweep cannot disable them: issue-labels, mobile-android-apk, mobile-showcase-screenshots, publish-aur, release, release-desktop, stale-needs-more-info, windows-tests. If a release edits one of them to add an outside event, the plan passes it. Only the later REQUIRE_NEW_OFF gate in fork-workflows.sh stops the branch move, and its advice is to move main by hand if the files look harmless. Fix: also check changed files and refuse when the new version has an outside event the old one lacked (or check every added or changed file GitHub does not list).
  • Ten old fork/* branches still carry mobile-eas-preview-fork.yml (for example fork/ci, fork/auto-update, fork/follow-upstream, fork/release-workflow, fork/fleet-runners). pull_request_target runs the base branch's copy, so an outside PR aimed at one of those branches still starts it, and the sweep cannot disable it. Today it fires only on labeled, with a job if on the label, which needs triage rights. Fix: delete the stale branches, or restrict PR base branches.

@scuttletoby @claude
fork: delete four outside-started upstream workflows and keep them de…
cf7f3b8 
…leted

The fork's sweep disables upstream workflows only once GitHub lists them, so it
cannot stop the first run of one an outside event starts. Owner decision
2026-11-12: delete mobile-eas-preview-fork.yml (pull_request_target),
desktop-macos-preview.yml, thread-transfer-report.yml and
mobile-fingerprint-label.yml (workflow_run on "CI", a name an outside PR can
take).

.github/fork-removed-workflows.txt lists them. fork-follow.sh reads it from
the series head (missing or malformed: the plan stops), resolves a rebase
conflict on listed files only by deleting them, deletes again any listed file
upstream brings back, refuses a same-base series that still has one, and
refuses a new upstream workflow that outside events can start (or whose
triggers fork-workflow-triggers.cjs cannot read) unless the list names it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scuttletoby

scuttletoby commented Nov 12, 2026

Copy link
Copy Markdown
Owner Author

lens: security S1 · reviewer: opus high (t3lens-14-s1) · read at: cf7f3b8

covered: the full diff of 919556a..cf7f3b8 and every function it calls in fork-follow.sh (fail, on_error, workflow_paths, new_workflows, refuse_upstream_fork_named, schema_check, latest_release, the advance/stage path); fork-workflow-triggers.cjs line by line; the fork-follow.yml plan job (checkout, env, token scope); fork-workflows.sh's unregistered/REQUIRE_NEW_OFF gate; the new tests in fork-follow.test.sh and fork-contracts.test.py. I formed my view before reading the PR body. Live state (read only): GitHub lists 19 workflows in the fork. The 4 deleted files are not among them. Of the files on main that can be started from outside, the listed ones (desktop-macos-preview-publish, pr-limits, pr-size, pr-vouch, relay-preview-cleanup) are all disabled_manually. 8 upstream files on main were never listed: issue-labels, mobile-android-apk, mobile-showcase-screenshots, publish-aur, release-desktop, release, stale-needs-more-info, windows-tests. Today none of them has an outside trigger. has_wiki=true, has_issues=false, has_discussions=false.

checks:

  • Tests in a scratch clone with GH_TOKEN and GITHUB_TOKEN unset. fork-follow.test.sh at cf7f3b8: all 125 checks pass. The same tests against main's scripts (FORK_FOLLOW_DIR): 18 fail, which matches the PR body. fork-contracts.test.py: all pass at the head; on main's tree with the new test file, 3 fail (list missing, the four not listed, the "listed file still in tree" mutant).
  • Hand mutations, one at a time, against the head scripts. Each of these is caught: dropping the "every unmerged path is listed" check (4 fail) and dropping the re-delete commit (2 fail). The ones the tests miss are in finding 3.
  • Parser bypasses tried. Each of these makes the reader stop (exit 2): flow on: [a, b]; a quoted "on" key; on : x (space before the colon) with no other on:; ? on complex keys; on: as a string; anchors, aliases and tags; a << merge key under on; a list at the same indent as on:; %YAML directives and several documents; a BOM; lone CR line ends; a symlinked workflow file; a missing on:; escaped event names. A true: key alone has no top-level on:, so it stops too. A .yaml extension is checked. A subfolder of .github/workflows is never run by GitHub. Renames show up as A because workflow_paths uses --no-renames, and names with a comma or a newline are refused there. A workflow_call target is reached only through a caller, which is disabled or deleted. Composite actions and upstream reusable workflows that fork-release.yml calls are the existing trust in upstream code, unchanged by this PR. The one parser bypass I found is finding 1.
  • Rebase auto-resolve: it continues only when every path from git diff --diff-filter=U is on the list. Any unlisted path, or a stop with no unmerged paths (for example an untracked file in the way), breaks out to rebase --abort. The step count is bounded by series_count. A rename/delete conflict lands on the new, unlisted name, so it is refused (tested). It never deletes an unlisted file. One small thing: git rm treats paths as pathspecs, so a listed name with glob characters ([ab].yml) would also match siblings. The list is owner-controlled, so not a finding. --literal-pathspecs would close it.
  • List check: / is excluded from the name, so no .. traversal. fork-* names are refused. Blank and comment lines are skipped. CR is stripped. Leading or trailing spaces and a missing file stop the plan. A list with comment lines only deletes nothing (by design, tested). The contract test's regex matches the script's.
  • Code execution with the push token: nothing from upstream is required, sourced or evaluated. node runs only the fork's own fork-workflow-triggers.cjs, whose only import is node:fs. An upstream file with the same path would be an add/add conflict on an unlisted path, so the plan refuses it. Upstream YAML is read with plain regexes (no backtracking hot spots). Upstream text reaches the report only in the middle of a line (newlines become spaces, path names with a newline are refused), so it cannot start a :: workflow command on stderr. Event names are checked against [a-z_]+ before they reach $GITHUB_OUTPUT. The token stays scoped as before (actions: write, and FORK_PUSH_TOKEN only in the push steps).
  • State: what happens when this runs again, stops half-way, or meets existing, old or empty state?
    • A re-run makes the same tree. The resolved commits and the re-delete commit are deterministic, and the "advance to an earlier release by tree" path is tested.
    • A red stop aborts the rebase and pushes nothing, so the next run stops the same way.
    • A rebase left on disk from an interrupted run: the plan always starts in a fresh git init checkout, so that never happens in CI. Run by hand in a dirty clone, the loop could go on with a stale rebase whose conflicts are all listed. That is a local-only edge case.
    • An empty list deletes nothing. A missing or malformed list stops the plan.
    • Upstream renaming a listed file is refused and names the new name. Upstream deleting the file and adding it back gets the re-delete commit (tested).
    • Same base with a listed file still in the tree is refused.
    • An old queued follow: the PR body says it "uses that commit's script". It does not: the plan job checks out SERIES_BRANCH (fork-follow.yml:63 plus the Checkout step), so every queued run uses main's current fork-follow.sh, including this logic. That is safer than the body claims. Please correct the body.

finding: minor .github/scripts/fork-workflow-triggers.cjs:49 the reader takes the first column-0 on: line as the trigger key. It does not check whether that line sits inside a multi-line quoted string or flow sequence. Example input: name: "x / on: push / " / on : issues / jobs... The reader prints push and exits 0. The backstop grep at fork-follow.sh:202 only looks for pull_request_target and workflow_run, so an issues (or issue_comment, discussion, watch, fork) workflow passes the check. PyYAML and js-yaml 4.1.0 both read that file as on=issues; the strict yaml 2.6.1 rejects it, because the column-0 continuation is not valid YAML. The same works with a flow list under strategy.matrix. Whether GitHub's parser accepts it is unknown, and upstream would have to merge such a file. Fix: stop when a column-0 line that is not a comment, --- or ... is not a plain key: (or "on":/'on':), or when a line leaves a quote or bracket open. That also refuses on : x and escaped keys outright. (confidence: real gap in the reader; medium-low that GitHub would run such a file)
finding: minor .github/scripts/fork-follow.sh:192 refuse_outside_workflows only checks files upstream adds (A). Today 8 upstream workflow files on main have never been listed by GitHub, so the sweep cannot disable them (for example stale-needs-more-info.yml and release.yml). Say upstream edits one of them to add issues: or pull_request_target. That is an M change, so the plan passes. Only the REQUIRE_NEW_OFF gate in fork-workflows.sh stops the branch move, and its advice is "if they are harmless, move main yourself". It does not name the outside trigger, so a person could move main, and the first outside event could then run the file before the sweep can see it. That is still a stop, but it rests on a person's judgment, not on the plan's rule. Fix: also check M files and refuse when the target version has an outside event the base version lacked (or check every AM file GitHub does not list). (confidence: high on the code path; the trigger needs an upstream edit like that)
finding: minor .github/scripts/fork-follow.test.sh (Round 5) several fail-closed checks can be deleted and all 125 checks still pass, so a later edit could drop them unseen. I deleted each one by hand: the grep -qwE 'pull_request_target|workflow_run' backstop (fork-follow.sh:202), the CR strip on list lines, the reader's "on: appears twice", "more than one YAML document", "indented with a tab", "event list over several lines" and "unknown form under on:" checks, and |/> in the refused on: forms. The rest also catch nothing: only "no on:" and a flow mapping test the reader's refusals. Add one case for each, as the contracts test does with broken copies. (confidence: high, measured)
finding: minor .github/scripts/fork-follow.sh:189 outside_events has no gollum. The fork has has_wiki=true. If wiki editing is open to everyone (a settings toggle; the default is collaborators only), a new upstream workflow on: gollum could be started by anyone. It costs one word to add. pull_request_review and pull_request_review_comment run the PR's own copy, like pull_request, so they are not needed here. (confidence: low that it matters today)

blockers: none. Safe to merge as it stands. Finding 1 is cheap to close in this PR, and finding 2 can be a follow-up.

@scuttletoby

scuttletoby commented Nov 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner Author

lens: security S2 · reviewer: opus high (t3lens-14-s2) · read at: cf7f3b8

covered: the full diff (fork-removed-workflows.txt, fork-follow.sh, fork-workflow-triggers.cjs, fork-contracts.test.py, fork-follow.test.sh, fork-follow.yml) and what it calls or feeds: fail, on_error, workflow_paths, new_workflows, refuse_upstream_fork_named, schema_check, latest_release, the plan/push/sweep steps in fork-follow.yml, and fork-workflows.sh (NEW_WORKFLOWS, REQUIRE_NEW_OFF). I also listed the fork's registered workflows (gh api, read only) and checked every branch in the fork for the deleted files.

checks:

  • (1) Parser bypasses. Each of these fails closed (exit 2): flow on: [a, b], quoted "on":/'on':, on: as a string, true:/On:/on :/? on (no on: found), multiple docs, %YAML directives, anchors and aliases, <<, tags, block scalars, a list split over lines, tabs, an indented root, and on: followed by a list at column 0. Comments and issues: under permissions/jobs are handled correctly. .yaml files are covered. A file in a subfolder is also parsed, which is harmless because GitHub ignores those. Composite actions and workflow_call chains only run through a caller, and upstream callers are disabled or checked. Upstream composite actions in fork-tests/fork-release were already trusted before this PR. Exceptions are in the findings below.
  • (2) Conflict auto-resolve. It only runs when every U path is on the list (break 2 otherwise, then abort plus red; tested). git rm acts only on those paths. Changes in the same commit that do not conflict stay staged. Skip vs continue is right for a resolution that leaves the commit empty. The loop is limited to series_count+1. If the stop is not a conflict (no U paths), the loop breaks, then aborts and goes red. A rename/delete conflict is reported on the new name and refused (tested).
  • (3) List check. The bash and python regexes agree. / is not allowed, so no traversal. fork-* lines, blank and comment lines, CRLF, and leading or trailing spaces are handled (a line with spaces is refused, which fails closed). A list with only comment lines is fine, and the empty array is safe under set -u. A missing list goes red.
  • (4) Code execution. node runs the fork's own .cjs from the candidate tree. If upstream touched that path, the rebase would conflict on an unlisted file and refuse. The .cjs uses only node:fs and makes no require or eval of upstream content. Its regexes are linear. Output is limited to [a-z_]+. Upstream text reaches the report only after fixed prefixes, so it cannot inject a workflow command.
  • (5) Tests. At the PR head, fork-follow.test.sh (125 checks) and fork-contracts.test.py pass. With the PR's tests run against main's scripts and tree, 18 and 3 checks fail, matching the PR body. I broke 13 parts by hand. These were caught: the conflict loop, the unlisted-path break 2, the re-delete commit, the same-base refusal, the fork-* rule, and the refuse_outside_workflows call. These were not caught (see the last finding): dropping the duplicate-on: check, the multi-document check, the tab check, the CR strip in the list, and the grep -w pull_request_target|workflow_run belt. The anchor and << guards turned out to be backed up by the EVENT regex.
  • State. A re-run is deterministic (same tree, advance tested). A half-way stop leaves nothing pushed. A leftover rebase is aborted, or the job's checkout is thrown away if set -e exits inside the loop. An empty or missing list: comment-only goes on, missing goes red. Upstream renaming a listed file is refused, or goes through the general check if git sees a delete plus an add. NEW_WORKFLOWS still names listed files that upstream edited, but fork-workflows.sh only counts files in the tree, so REQUIRE_NEW_OFF does not block the move forever. One correction to the PR body: an old queued follow does not run its own commit's script. The plan job checks out SERIES_BRANCH, so it runs the current script. The result is the same.

finding: minor .github/scripts/fork-workflow-triggers.cjs:29 The parser fails open (exit 0, prints push) on YAML that a 1.2 parser reads as having outside triggers. (a) A lone CR line break: on:\n push: {}\r issues: {}\n. (b) A NEL (U+0085) line break, which YAML 1.1/libyaml treat as a newline. (c) A decoy on: [push] inside a multi-line double-quoted name: "..." with the real key escaped as "on": [issues]. (d) A duplicate key on: [push] + "on": [issues], if GitHub keeps the last one. In (c) and (d), escaping the event name too (pull_request_target) also gets past the grep belt. I confirmed all four with PyYAML (bool resolver off). This needs a hostile upstream, and REQUIRE_NEW_OFF plus the sweep still back it up, so it is minor. But it breaks "fails closed on anything it can't classify". Fix: refuse any \r not followed by \n, U+0085, U+2028 and U+2029, any top-level key that starts with " or ' and contains \, and any multi-line quoted scalar at the top level. (parser behaviour high; GitHub's handling of CR and escaped keys medium)
finding: minor .github/scripts/fork-follow.sh:192 refuse_outside_workflows only checks added files (workflow_paths A). Eight upstream files in the fork are not registered by GitHub, so the sweep cannot disable them: issue-labels, mobile-android-apk, mobile-showcase-screenshots, publish-aur, release, release-desktop, stale-needs-more-info, windows-tests. If an upstream release edits one of them to add issues: or pull_request_target:, the plan passes it. Only the later REQUIRE_NEW_OFF gate stops the move, and its advice is "if harmless, move by hand". Use AM (as new_workflows does), or check files that are added or changed and not registered. (high)
finding: minor .github/scripts/fork-follow.sh:189 outside_events is missing pull_request_review and pull_request_review_comment. Any GitHub user can review a same-repo PR, and that runs the PR branch's copy with the repo token. gollum is also missing; it matters if the fork's wiki is open. A new upstream workflow on these events passes the early check, and only REQUIRE_NEW_OFF holds it. (medium)
finding: minor .github/workflows/mobile-eas-preview-fork.yml (deleted only on main) Ten other fork branches still have it (fork/ci, fork/auto-update, fork/follow-upstream, fork/release-workflow, fork/fleet-runners, ...). pull_request_target runs the base branch's copy, so an outside PR aimed at one of those branches still starts this file, and it is not registered, so the sweep cannot disable it. Today it only fires on labeled with a job if on the label, which needs triage rights, so outsiders alone cannot start it. Deleting the stale fork/* branches, or restricting PR base branches, closes this. thread-transfer-report and mobile-fingerprint-label use workflow_run, which runs only from the default branch, so they are not affected. (medium)
finding: minor .github/scripts/fork-follow.test.sh:490 Parser checks that fail open when removed have no test. With the duplicate-on: check deleted (cjs:50), the full suite still passes; it then takes the first on:. The same is true of the multi-document check (cjs:45), the tab check (cjs:30), the list's CR strip (fork-follow.sh:168, which fails closed anyway), and the grep -w belt (fork-follow.sh:202). Add one plan case for each. (high)

@scuttletoby
fork: the trigger reader fails closed on more shapes and has its own …
95a2264 
…tests

- fork-workflow-triggers.cjs exports readTriggers and gets
  fork-workflow-triggers.test.cjs, like the other fork-*.cjs scripts. The
  test requires it, so knip no longer reports it as an unused file.
- The reader refuses: a line break other than \n or \r\n (a lone \r, U+0085,
  U+2028, U+2029); an unindented line that is not a plain `key:` (`on :`,
  escaped or quoted keys, `? on`, directives, a root list, a BOM); a quote
  left open at the end of a line; a bracket list whose later lines are not
  indented more than its first, or that never closes. Block scalar text is
  skipped. Indent counts ASCII spaces only.
- Outside events add pull_request_review, pull_request_review_comment,
  gollum, check_run, check_suite, status and deployment_status.
- fork-follow.test.sh: plan cases for a CRLF list, the plain-text
  pull_request_target backstop, the new events and a decoy on: in a
  multi-line string.
- fork-schema-check.test.cjs: drop an unused parameter that vp check
  reports as an error.
@scuttletoby scuttletoby reopened this Nov 12, 2026
@scuttletoby

scuttletoby commented Nov 12, 2026

Copy link
Copy Markdown
Owner Author

lens: correctness+security r2 · reviewer: opus high (t3lens-14-r2) · read at: 95a2264

covered: the full fix diff cf7f3b8..95a2264 (fork-workflow-triggers.cjs rewritten, the new fork-workflow-triggers.test.cjs, outside_events and new cases in fork-follow.sh / fork-follow.test.sh, fork-ci-tests.txt, the lint fix in fork-schema-check.test.cjs). I also read what it calls into: refuse_outside_workflows, workflow_paths, the list reader (CR strip, removed_re) and the fail path. I checked each S1 and S2 round-1 finding against the new code and the PR body's Follow-ups section.

checks:

  • Round-1 findings, each fixed at the root and failing closed:
  • Tests, with GH_TOKEN and GITHUB_TOKEN unset, in a scratch clone:
    • At the head: fork-workflow-triggers.test.cjs passes 13/13 and fork-follow.test.sh passes all 131 checks.
    • Against cf7f3b8's reader (through a small adapter, because the old file exported nothing): 6 of the 13 tests fail. Those are odd line breaks, non-plain unindented keys, the open quote, the bracket-indent rule, a block scalar's later lines, and forms under on:.
    • fork-follow.test.sh with FORK_FOLLOW_DIR set to cf7f3b8's scripts: 4 fail (pull_request_review, pull_request_review_comment, gollum, and the decoy quote case).
  • Hand mutations, one at a time, at the head. Each of these turns at least one test red:
    • In the reader: the ODD_BREAK, tab, open-quote, bracket-indent, never-closed, TOP_KEY, on: twice, multi-document, \ escape, quote-only-at-token-start, uneven-indent, empty on:, <<, EVENT regex and quoted-ON_KEY checks.
    • In fork-follow.sh: the CR strip, the grep -w belt, issues, pull_request_review(_comment) and gollum.
    • Five reader mutants survive: allowing a closing ] less indented than its opener (only accepts more), opening [ inside a bracket without a token start (no change in practice), treating any # as a comment (only refuses more), and dropping the '' escape inside single quotes (see the finding). Dropping check_run, check_suite, status and deployment_status from outside_events also survives (see the finding).
  • The multi-line bracket rule, normal workflows versus tricks:
    • I ran the reader on all 369 distinct workflow blobs in the fork's and upstream's history and compared the result with PyYAML (bool resolver off). All 369 match exactly, none is refused, and every workflow on upstream main reads correctly.
    • The rule cannot hide an outside trigger, because it only skips lines indented more than the line that opened the bracket. A line at column 0 can never be skipped (only a lone closing ] may line up with its opener), so YAML's top-level on key is always checked by TOP_KEY and ON_KEY.
    • Under on:, the on: line cannot open a bracket or a block (on: [.. over lines, {, &, | and > are all refused), and any child at the child indent is less indented than any opener after it. A misread under the rule therefore only goes the safe way: it refuses the file (for example ], or a closing bracket less indented than its opener), or it names an event YAML would not see.
    • Probes I tried: an anchor or tag before an open quote hiding a decoy on: gives push where YAML has no on at all, and GitHub would reject that file. Adding a real "on": [issues] is refused as on: twice. A quote opened under on: that YAML treats as text gives an extra event (safe). So no normal workflow can be misread into a missed trigger. Only a deliberate trick can make the reader wrong, and the checks above then refuse it.
  • State: what happens when this runs again, stops half-way, or meets existing, old or empty state?
    • The reader is a pure function of the file, so a re-run gives the same answer.
    • A plan that stops half-way pushes nothing and aborts the rebase in the job's fresh checkout, as in round 1.
    • Only files a release adds (A) reach the reader, so the 26 existing upstream files are never re-judged, and all of them parse today anyway.
    • A new upstream file in a style the reader refuses (a multi-line quoted if:, a ], line) stops the follow red until a person lists it or rebases by hand. That fails closed, at the cost of availability.
    • An empty list deletes nothing (tested). A missing list goes red. A CRLF list now reads like an LF one (tested). An upstream rename of a listed file is refused (unchanged and tested).
    • A follow queued before the merge runs main's script. The PR body has been corrected to say so.

finding: minor .github/scripts/fork-workflow-triggers.cjs:42 A quote after an anchor or tag (name: &a "x, !t 'x) is not seen as opening a string. The reader then misses an open multi-line quoted scalar. Also, dropping the '' escape rule at :34 still passes every test. Today neither can make the reader miss a trigger: TOP_KEY plus the "on: twice" check refuse any second real on. Still, it is the same class of gap S2 flagged, and the open-quote guard is meant to be a guard of its own. Fix: set tokenStart after an &anchor / !tag token followed by a space, and add refuses(yml("name: &a \"x", "on: push", "\"")) and refuses(yml("name: 'it''s", "on: push", "'")). (high on the behaviour, low impact)
finding: minor .github/scripts/fork-follow.test.sh:498 Only 7 of the 15 outside events have a plan case. I deleted check_run|check_suite|status|deployment_status from outside_events and all 131 checks still pass. watch, fork, discussion and discussion_comment are also untested, so a later edit could drop any of them unseen. Fix: loop over every event in outside_events, or add one case each (printf 'on: [check_run]\n' and so on). (high, measured)

blockers: none. The round-1 reader findings are fixed at the root and fail closed. The open items are the two minors above and the two follow-ups the PR body already lists.

@scuttletoby
scuttletoby marked this pull request as ready for review November 12, 2026 17:58
@scuttletoby
scuttletoby merged commit f72c880 into main Nov 12, 2026
26 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@scuttletoby